Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added si5351 driver #474

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Added si5351 driver #474

wants to merge 4 commits into from

Conversation

conotto
Copy link

@conotto conotto commented Oct 19, 2022

Driver for Si5351 clock module

@deadprogram deadprogram changed the base branch from release to dev October 27, 2022 09:58
@deadprogram
Copy link
Member

Hello @conotto thank you for working on this driver.

I have changed the branch to dev as described here: https://github.com/tinygo-org/drivers/blob/release/CONTRIBUTING.md#how-to-use-our-github-repository

@conotto
Copy link
Author

conotto commented Oct 27, 2022

Please let me know if anything is missing

si5351/registers.go Outdated Show resolved Hide resolved
Co-authored-by: Nils Stinnesbeck <[email protected]>
@conotto
Copy link
Author

conotto commented Dec 16, 2022

Is there anything else required ? Or can this be merged already ?

Copy link

@rminnich rminnich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting to see this driver, thanks!

lastRdivValue [3]uint8
}

var errNotInitialised = errors.New("Si5351 not initialised")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that nowadays, the practice is to make these exported:
var Err...


// Configure sets up the device for communication
// TODO error handling
func (d *Device) Configure() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that Configure does not have an error return


// Disable all outputs setting CLKx_DIS high
data[0] = 0xFF
d.bus.WriteRegister(d.Address, OUTPUT_ENABLE_CONTROL, data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this get an error? I'm guessing yes?


// Connected returns whether a device at SI5351 address has been found.
func (d *Device) Connected() bool {
err := d.bus.Tx(uint16(d.Address), []byte{}, []byte{0})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can Tx get an error that is not just about things not being found? It seems a bit dangerous to throw the error away, or is that the interface definition? I"m still catching up.


func (d *Device) EnableSpreadSpectrum() (err error) {
data := d.buf[:1]
err = d.bus.ReadRegister(d.Address, SPREAD_SPECTRUM_PARAMETERS, data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more common practice, at least at google, would be an unnamed error return and then
if err := d.bus.ReadRegister(...); err != nil {
return err
}
...
return d.bus.WriteRegister
it avoids any possible problems around shadowing (which has caused trouble in practice)

// Reset both PLLs
data = d.buf[:1]
data[0] = (1 << 7) | (1 << 5)
d.bus.WriteRegister(d.Address, PLL_RESET, data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this error? Do you want to return an error if it does?

}
// Channel range
if !(output < 3) {
return errInvalidParameter

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's another case where more informative errors might be useful, viz:
return fmt.Errorf("output %d is < 3:%w", ErrInvalidParameter)

switch output {
case 0:
baseaddr = MULTISYNTH0_PARAMETERS_1
break

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need these breaks. That's only an issue in C.

data[6] = uint8((p2 & 0xFF00) >> 8)
data[7] = uint8(p2 & 0xFF)
err = d.bus.WriteRegister(d.Address, baseaddr, data)
if err != nil {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, strongly recommend the practice:
if err := d.bus.Write(...); err != nil {
return nil
}

avoid named return parameters is usual.

switch output {
case 0:
register = CLK0_CONTROL
break

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need the breaks.

btw, golangci-lint would likely make useful suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants